Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Grabmayrs gamma-cascade-reader and a custom neutron capture process #123

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

EricMEsch
Copy link
Contributor

The first commit is intended to add a refurbished version of the gamma-cascade-reader originally written by @MoritzNeuberger for WLGD. It employs a custom RMGNeutronCaptureProcess inheriting from G4HadronicProcess and replacing the native G4NeutronCaptureProcess. This process is supposed to employ the gamma cascades from Peter Grabmayr (https://link.springer.com/article/10.1140/epjc/s10052-023-11602-y) upon neutron capture on specified Isotopes, otherwise keeping the standard process. It manually employs the cross section datasets that are usually employed with Shielding and takes over all of the interactions from the native G4NeutronCaptureProcess. Works with 3 of the 4 physics list, as QGSP_BERT_HP does employ a different process for "nCapture" (although this could be fixed with some investigation, for now this will result in just the native process being used and an error message thats not crashing the application, if the UseGrabmayrsGammaCascades macro is set).

There are certainly other options for this feature, but overall to me this seems like a "smoother" solution, than doing it in the tracking action like in WLGD. However, if there are other ideas that suit the concept of ReMaGe more, i am open ears. This is just a proposal.

The second commit provides an example to test this new feature, while employing a user-defined optional output scheme to write out all of the isotopes created through nCapture or RMGnCapture. It provides two macros to compare the native result to this new custom version.

The example does not need to be pulled, but it allows for quick tests (and shows how a user-defined optional output scheme would work)

@ManuelHu
Copy link
Collaborator

ManuelHu commented Sep 2, 2024

Did you test this with MT mode on? I do not suppose that all the file-reading stuff is thread-safe, or is it? (making the reader instance thread-local should be enough. It has to be constructed on each thread anyway, otherwise the registered commands will not really work in some edge cases.)

@EricMEsch
Copy link
Contributor Author

I did test it, thats why in the UserAction the instance is called in the Build() and BuildForMaster() as it seems in Single thread mode it requires it to be in Build() and in the multi threaded case it seems to work with the instance first being called in BuildForMaster().

This worked for my tests, but i have to admit i am not too sure if that solution works for all edge cases, as i am not entirely sure about the interaction of singletons in MT mode.

@ManuelHu
Copy link
Collaborator

ManuelHu commented Sep 2, 2024

I do not have time to test it now, but I would guess that the current code will fail if you move /RMG/GrabmayrGammaCascades/SetGammaCascadeFile commands after /run/initialize.

I would be more concerned with only using one global instance without explicit locking, when it comes to file reading. Afaik, the C++ standard does not guarantee thread-safety around stream objects (see section 30.2.3 in the C++ 17 draft)

I did test it, thats why in the UserAction the instance is called in the Build() and BuildForMaster() as it seems in Single thread mode it requires it to be in Build() and in the multi threaded case it seems to work with the instance first being called in BuildForMaster().

I think, BuildForMaster() will not be called in single-threaded mode; only Build() is called...

The initialization will be mostly work fine, even though technically it is UB, I suppose.

Copy link
Collaborator

@ManuelHu ManuelHu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should work (not compile-tested though).

src/RMGGrabmayrGCReader.cc Outdated Show resolved Hide resolved
include/RMGGrabmayrGCReader.hh Outdated Show resolved Hide resolved
src/RMGUserAction.cc Outdated Show resolved Hide resolved
@EricMEsch
Copy link
Contributor Author

This fail seems to be totally unrelated to the changes commited? (During runtime all of the added code is inactive if the macro is not set)

@ManuelHu
Copy link
Collaborator

ManuelHu commented Sep 3, 2024

This fail seems to be totally unrelated to the changes commited? (During runtime all of the added code is inactive if the macro is not set)

Yes, it's just the typical test being flaky, as G4 11.0 has apparently some navigation bugs :-)

src/RMGGrabmayrGCReader.cc Outdated Show resolved Hide resolved
src/RMGGrabmayrGCReader.cc Outdated Show resolved Hide resolved
src/RMGGrabmayrGCReader.cc Outdated Show resolved Hide resolved
src/RMGGrabmayrGCReader.cc Outdated Show resolved Hide resolved
@ManuelHu
Copy link
Collaborator

Now it really looks better. I still have some minor (mostly cosmetci) remarks, then it should be good from my side :-)

src/RMGGrabmayrGCReader.cc Outdated Show resolved Hide resolved

RMGGrabmayrGCReader::GCMessenger::GCMessenger(RMGGrabmayrGCReader* reader) : fReader(reader) {
fGammaFileCmd = new G4UIcommand("/RMG/GrabmayrGammaCascades/SetGammaCascadeFile", this);
fGammaFileCmd->SetGuidance("Set the Z, A and /path/to/file for the gamma cascade employed upon "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not really like the message here. It seems to technical for a general description of what this command does. Maybe something akin "Set a gamma cascade file to be used upon neutron capture on a specifc isotope"?
(The parameters are already described by their own docstring, thats fine.)

src/RMGPhysics.cc Outdated Show resolved Hide resolved
src/RMGPhysics.cc Outdated Show resolved Hide resolved
src/RMGNeutronCaptureProcess.cc Outdated Show resolved Hide resolved
src/RMGGrabmayrGCReader.cc Outdated Show resolved Hide resolved
@EricMEsch
Copy link
Contributor Author

EricMEsch commented Sep 12, 2024

I tried to group a manual edit fix together with adding the suggestions in one commit purely on the web-interface, which i guess didnt work^^ (I have to redo the doc-dump anyways so)

@EricMEsch
Copy link
Contributor Author

Should be fine now. Anyways, thanks for the feedback and putting up with my code :)

@ManuelHu ManuelHu merged commit 44b4d4a into legend-exp:main Sep 12, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants